fix(studio): target one element when adding a keyframe at the playhead - #2847
Conversation
…me percentages A ruler press with no pointer movement settled the playhead at t=0 instead of the clicked time. handlePointerUp replays pendingClientXRef, which only the pointermove path wrote, so a plain click fell back to the ref's initial 0 and overwrote the correct pointerdown seek. Seed the ref on pointerdown. The keyframe retime move branch also returned the raw quotient while the resize branch rounded to 3dp, so values like 74.81203007518799% landed in the user's source and churned the diff on every drag. Round at the point of computation so the no-op test and the written value agree.
The 24x24 WCAG 2.5.8 overlay sits on a wrapper that outranks the diamonds, so on a segment narrower than 24px it overhung them and won their hit test at fit zoom. Gate the overlay on the clear span between the two diamonds and let the button keep its 16x16 box below that. Also renames the pointer target suite to say it asserts the classes that produce the size, not the measured geometry, which happy-dom cannot see.
…ide press Split hex-draft ownership so the hex input is the sole author of its own text while editing (updateColorDraft no longer stamps a canonical hex back over every keystroke), fixing snap-back on backspace and the silent wrong-colour clobber on non-repeating hex values. Route hex typing through the shared gesture transaction so outside-click and Escape settle/cancel it like the other inspector fields, instead of relying on a private onBlur commit that never fires once the panel unmounts on outside-click.
Swap the panel's hand-rolled bubble-phase mousedown listener for the shared useContextMenuDismiss hook, which adds Escape support and fixes outside-click dismissal when a canvas gesture (e.g. marquee start) calls preventDefault on pointerdown, which otherwise suppresses the mousedown compat event entirely. Also wires up dialog ARIA (role, aria-modal, id/aria-controls) between the trigger and panel.
The panel does not trap focus and leaves the rest of the editor operable, so aria-modal would tell assistive tech the whole app is inert while it is open. role=dialog plus aria-controls and Escape is the correct non-modal disclosure shape.
The gesture resolver gated on six digits while parseCssColor accepts both lengths, so #F00 previewed as nothing and committed nothing. The old onBlur path parsed it, making this a behavioural loss rather than a pre-existing gap. Also asserts that an incomplete hex is restored on outside-click, not merely left uncommitted.
A sub-composition tween's resolvedStart is composition-local, while the timeline element resolved for it is the sub-comp HOST, whose start is main-timeline absolute. toClipPercentage subtracted the two frames from each other, so a host mounted at 1.5s cached its 0s tween at -12% and its last tween's end keyframe at 88% instead of 100%. A clip-relative percentage can never be negative. resolveClipTimingBasis now returns the clip start in the frame the tween's own times are measured in: the composition mount (expandedParentStart for an expanded child, the parent composition clip's start otherwise, 0 for a root-composition element) is subtracted, and a sub-comp inner element that falls back to its host's window starts at 0 in that window. It moves to gsapShared so the post-commit cache writer can share it instead of resolving its own basis, which also gives that writer the sub-comp host fallback it was missing.
buildExpandedElements synthesized DOM-only sub-composition children against the top-level element rather than the parentHost it resolves immediately after. Under two-level nesting every child row therefore inherited the top-level window instead of its own host's, so the rows drew at the wrong offset and duration.
The sub-composition drill-in replaced the host row with its children. Since expansion is also driven by the playhead alone (paused auto-expand), an ordinary seek into a sub-composition made the host row disappear, taking its keyframe lane with it: diamonds render per row from keyframeCache.get( elementKey), so no row means no diamonds. Reproduced live with no drag at all, seek 0 gave 3 diamonds, seek 7.68 gave 0, seek 0.2 gave 3. Make the expansion additive instead. The host row stays and its children are appended directly below it. The synthetic fractional lanes already used for children sit strictly between the host's lane and the next integer, so the host keeps its own row without colliding with anything. The time-keyed auto-expand itself is unchanged.
61a7d36 to
00df2e3
Compare
ff16b42 to
295f98b
Compare
Drilling two levels deep spared only the top-level row, so the middle host lost its row and its keyframe lane with it. Spare every host between the drilled one and the top, and anchor the children under the deepest host that actually has a row. Also stops resolveClipTimingBasis handing back a main-timeline start when a clip names a parent composition that is absent from the element list. The mount is unknowable there, so the child's own window is the only safe frame.
Two halves of one inversion in the expanded timeline lanes: the tweens
that should show were filtered out, and a tween that should not be there
was the only survivor.
Lane classification read the parser's whole-tween verdict, which is
undefined for anything spanning more than one property group. `{x,
opacity}` is the canonical HyperFrames entrance tween, so five of the
seven tweens in the swiss-grid graphics example had no caret, no
reserved row and no diamonds. Classify per property instead, through one
helper both the rendered lanes and the reserved row heights count
through so they cannot drift again.
Attribution matched an unanchored leading id, so `#stat3 .block` was
filed under `#stat3`. The child's diamonds landed on its ancestor and
collided with the ancestor's own tween at the shared percentage, which
the same-percentage merge then resolved by dropping the ease. Route
attribution through resolveSelectorElementIds, which anchors a
whole-selector id and otherwise resolves through the live preview DOM,
and anchor its no-DOM fallback so a descendant selector resolves to
nothing rather than to its ancestor. The merge rule is unchanged.
Also brings the last property-lane call site onto the shared clip timing
basis: an expanded sub-composition child's start is host-absolute while
its tweens are local to its own file.
00df2e3 to
59a818e
Compare
295f98b to
37140af
Compare
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 37140af.
Clean, well-scoped fix for a real data-loss bug. writeTargetSelector's rung ladder (#id → [data-hf-id] → already-unique own selector → :nth-child structural → bare-selector fallback) matches the identity semantics the runtime uses for read-back, and the new gsapShared.writeTarget.test.ts exercises the actual write→parse→attribute round trip against a real jsdom + parseGsapScript + addAnimationWithKeyframesToScript, which is exactly the shape that failed before. The 5-siblings collapse regression is directly asserted.
CI note — the failing preview-regression gate is CI infra noise, not a real regression. PREVIEW_PARITY_RESULT: cancelled (preview-parity got cancelled, likely superseded by a rebase) cascaded into the gate treating != "success" as failure — same shape as [[feedback_ci_shard_cascade_noise]]. A retrigger should clear it.
Cross-cutting theme in the inlines: the PR body explicitly scopes the fix to two writers (useEnableKeyframes no-anim branch + commitKeyframeAtTimeImpl) and defers six more to a top-of-stack sweep. That's fine as a strategy, but three other paths inside the same file — the replace-with-keyframes mutations at useEnableKeyframes.ts:156, 262, 361 — do the same bare-selector write, and the PR body's rationale ("retargeting a tween the author aimed at a whole group is a different decision from adding a keyframe") applies to authored group-tweens but not obviously to the case where the target was already idless. Worth clarifying whether those three are in the deferred-follow-up bucket or genuinely intentional.
Nothing blocking. LGTM from my side.
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
R1 review — writeTargetSelector decouples write-target from match-target
Framing. The new-tween write path was authoring through selectorFromSelection, the same helper the match path (findTweenAtTime) needs to keep returning its exact string. For an id-less element that helper returns whatever buildStableSelector produced — a bare class like .group — so authoring against 1 of 5 siblings wrote a tween GSAP resolved to all 5 and that resolveSelectorElementIds read back as all 5, collapsing their timeline rows into one. writeTargetSelector is the write-side counterpart: #id → [data-hf-id="…"] → the selection's own selector when already unique → a :nth-child chain anchored on the nearest identifiable ancestor, each rung verified against the live DOM. Two authoring sites are wired (the no-animation branch of useEnableKeyframes, and the add-with-keyframes mutation in commitKeyframeAtTimeImpl); the match-path use of selectorFromSelection in commitKeyframeAtTimeImpl is preserved and its comment names why, which is the load-bearing decision here. replace-with-keyframes intentionally still writes the selection's own selector — retargeting a whole-group tween is a different user decision. All correct in shape.
Findings
P2 — Ponytail fallback re-introduces the exact broken selector for the edge case the guard-comment says it excludes.
packages/studio/src/hooks/gsapShared.ts—writeTargetSelector, final line:return selection.selector ?? null;- The block-comment above says "it is only reachable with no live DOM to disambiguate against." But with
element && doctruthy we can still exit theif (element && doc)block when bothmatchesExactlyOne(doc, selection.selector, element)returns false ANDstructuralSelector(element)returns null.structuralSelectorreturns null in at least these live-DOM cases: element has noparentElement(root / detached / hosted across a shadow-root boundary, whereparentElementreturns null); or its finalmatchesExactlyOneon the built chain fails. The realistic one is detached-between-select-and-commit: user selects a class-only element, some code re-parents or removes it before the K press commits —element.ownerDocumentstill hands back a document but the walk short-circuits, and the ponytail returns.groupunchanged. That is a round-trip through the exact bug this PR is fixing. - Failure mode:
useEnableKeyframesalready has anif (!selector)no-op fallback tosession.handleGsapAddAnimation("to")— a graceful fallback, not a silently dead button. Returningnullwhen the DOM was present but structural failed would route through that same guard. Similarly,commitKeyframeAtTimeImpl's outerwriteTargetSelector(selection) ?? selectorwould fall back toselectorFromSelection— same broken bare class either way, but at least the branching invariant would be honest. - Suggested tightening: return
nullfromwriteTargetSelectorwhenelement && docwas true but structural failed. Keep the raw-selection.selectorreturn for the!element || !docbranch only, and update the comment to match. If the design intent really is "prefer a broken selector over a no-op," say so — but the current comment overstates the guard.
P2 — Test coverage gaps around the structural walk and the ponytail.
packages/studio/src/hooks/gsapShared.writeTarget.test.tscovers#id,data-hf-id, unique class, and 5-class-siblings-under-#scene(a two-step walk with the identifiable ancestor at depth 1). The round-trip test adds ids after selection to prove single-element attribution — good, that's the load-bearing behavioral assertion.- Missing: a deep chain with a mixed-identity ancestor (
#outer > div:nth-child(2) > [data-hf-id="mid"] > .child:nth-child(3)) — the unshift-and-break identity handoff is where a regression will hide. - Missing:
selection.selectorthat is invalid CSS (digit-leading id, unbalanced bracket) —matchesExactlyOne'stry/catchswallows the SyntaxError and the "fall through to structural" contract deserves a lock, especially sinceidSelectoralready exists to handle digit-leading ids elsewhere in this file. - Missing: the ponytail path itself. Whatever the resolved behavior is (per finding 1), the current suite has no assertion covering "structural returns null with a live DOM."
P2 — commitKeyframeAtTimeImpl's outer writeTargetSelector(selection) ?? selector fallback is (effectively) dead.
packages/studio/src/hooks/gsapKeyframeCommit.ts,add-with-keyframesmutation:targetSelector: writeTargetSelector(selection) ?? selector.writeTargetSelectorreturnsselection.selector ?? nullin its ponytail and always returns a non-null string for the#id/data-hf-idbranches; the outerselectorisselectorFromSelection(selection), which isidSelector(selection.id) ?? selection.selector ?? null. Given we already returned early on!selector, the only way forwriteTargetSelectorto return null whileselectoris truthy isselection.idset with noselection.selectorand noelement/doc— but theselection.idbranch ofwriteTargetSelectorfires first and always returns. So?? selectoris defensive dead code today. Not a bug — a// keeps the invariant explicit even if writeTargetSelector loosens latercomment (or dropping it) would read cleaner.
Not-blockers, worth flagging
- Six more write sites reach the same defect (add-animation button, drag, resize, rotate, gesture recording, property panel), scheduled for the top of this stack. Fine — but each of those six needs its own "wrote a class-only sibling, read back one element, timeline row survived" behavioral test alongside the swap. A helper swap without that assertion is where the top-of-stack PR is most likely to miss a site.
replace-with-keyframesintentionally keepsselectorFromSelection. The design call is clear (retargeting a whole-group tween is a different user decision), but nothing locks the intent — worth a test that asserts the retarget path still produces the group selector so future refactors don't drift silently.- The
:nth-child"known ceiling" (a runtime that injects wrapper elements shifts the index against the authored source) is called out in the body. Acknowledged. structuralSelectorusesArray.prototype.indexOf.call(parent.children, node)— works fine, but[...parent.children].indexOf(node)reads cleaner. Nit.
Verdict
COMMENT. Right shape, right sites, correct preservation of the match-path selector, behavioral round-trip tests through the real writer. The ponytail-vs-guard-comment mismatch is the one worthwhile follow-up: it silently re-enables the bug in a narrow but plausible detached-element window, and the fallback path in useEnableKeyframes is already there to handle a null return safely.
Grade: B+ — correct and well-tested for the primary scenario; the ponytail edge case and a couple of walk-coverage gaps prevent an A.
— Review by Via
animIdForProp matched on the parser's whole-tween propertyGroup, which is
undefined for a legacy mixed tween such as {x, opacity}. Such a tween never
matched, so an edit to either property fell through to the selection's
default animation, a different tween than the lane the user is editing.
Resolve through animationLaneGroups, the same per-keyframe helper the
rendered lanes and the reserved row heights already count groups with.
"Add keyframe at playhead" on an element with no id authored the bare class
buildStableSelector hands back, so one add on a `.group` wrote
`tl.to(".group", ...)`: a tween that animates all five siblings and that
resolveSelectorElementIds reads back as all five, collapsing their timeline
rows into one. It survived a reload, so the written file stayed un-editable.
writeTargetSelector is the write-side counterpart to selectorFromSelection
(which must keep returning the exact string findTweenAtTime compares against).
It resolves the element's own identity to a selector that addresses exactly
one element: `#id`, else `[data-hf-id="..."]`, else the selection's selector
when it is already unique, else a `:nth-child` path anchored on the nearest
identifiable ancestor (the selector + selectorIndex pair, resolved through the
DOM the index was counted in).
Applied to the two paths that author a NEW tween: the no-animation branch of
useEnableKeyframes and commitKeyframeAtTimeImpl. replace-with-keyframes still
writes the selection's own selector, since retargeting a tween the author
aimed at a whole group is a different decision from adding a keyframe.
writeTargetSelector returned the selection's bare selector whenever the structural walk failed, including when a live DOM was there to check against. An element detached between selecting and committing takes that path, so the add re-authored the exact `.group` string the function exists to replace. Return null instead: a failed walk against a live DOM is evidence, not absence of it. Callers that cannot drop a user edit opt back in with `?? selectorFromSelection` where the trade is visible. The replace-with-keyframes paths had the mirror defect. The server deletes and re-adds the tween, so their target string is a full rewrite, and they derived it from the selection: promoting a set on a tween already narrowed to `#scene > div:nth-child(3)` widened it back onto every class sibling, undoing the narrowing an earlier add had made. They now keep the tween's own authored target, matching what eight sibling commit modules already do.
37140af to
f04cdb7
Compare
|
All findings from both reviews are addressed at Ponytail fallback (P2, both reviewers). Confirmed by probe, not by reading: detaching the element between select and commit leaves Test gaps (P2, Via). Four added: deep mixed-identity chain, invalid-CSS fall-through, the null-with-live-DOM path, and the no-DOM path that still returns the raw selector.
Six remaining writers. Still at the top of the stack (#2849), and your point about each needing its own behavioural assertion rather than a bare helper swap is well taken; that is how the sweep is tested there. Not taken: dev-only Not taken: a test locking the replace path to the group selector (Via). That behaviour turned out to be the bug, so locking it would have cemented the re-widening. The test locks the opposite invariant instead. Studio suite 3042 passing on this branch, typecheck / oxlint / oxfmt clean. |
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
R2 review — all three R1 P2s resolved; scope expanded to the mirror bug on replace paths
Reviewed at f04cdb79c5f1475fa7d6048036a4e0af354a60a2.
Delta since R1 (37140aff): force-push; single new commit f04cdb7 on top of fd5555b. packages/studio/src/hooks/gsapShared.ts more than doubled its additions (+50 → +105). New export existingTweenTargetSelector factored out; three call-sites in useEnableKeyframes.ts rewired; gsapShared.writeTarget.test.ts now covers 12 cases including the round-trip and the commitKeyframeAtTimeImpl end-to-end.
R1 finding verdicts
| R1 P2 | Verdict |
|---|---|
#1 — Ponytail fallback reintroduces the bug in writeTargetSelector |
✅ RESOLVED. The live-DOM branch (lines 260-264) now gates on element && doc and returns structuralSelector(element) directly — a failed walk against a live DOM returns null rather than falling through to the bare selector. The last-rung return selection.selector ?? null at line 266 is now only reachable with no DOM to disambiguate against. Detach reproducer in the test at :141 pins the corrected behavior. |
| #2 — Test coverage gaps | ✅ RESOLVED. Deep mixed-identity chain at :107 (walk stops at data-hf-id="mid", does not keep climbing to #outer). Invalid-CSS selection.selector fall-through at :131 (the exactly-one rung reads as a miss rather than crashing). Ponytail-path assertion superseded by the null-return case at :141 — which is what the corrected code produces. |
#3 — Dead ?? selector fallback in commitKeyframeAtTimeImpl |
writeTargetSelector returns null on a failed structural walk against a live DOM (the fix for #1), the ?? selectorFromSelection(selection) at gsapKeyframeCommit.ts:75 becomes reachable AND necessary — this is the add-with-keyframes path that has no graceful no-op to fall to, so it takes the author's own selector, group collapse and all, over dropping the keyframe. The inline comment at :71-74 makes the trade visible. Not a defect at the new head. |
Fresh adversarial pass at new head
existingTweenTargetSelectoraddition (gsapShared.ts:279) — the mirror bug Rames flagged on the three replace-with-keyframes paths. Gate is correct:animation.targetSelector && !animation.hasUnresolvedSelectorpreserves an already-narrowed tween; only an unresolved target falls back to the selection. Consumer sites (extendKeyframedTweenToPlayhead,promoteSetToKeyframes,replaceSetWithSingleKeyframe) all switched fromselectorFromSelection(sel)toexistingTweenTargetSelector(anim, sel)and continue to short-circuit on!selector.- Round-trip test (
writeTargetSelector — write/read round trip, :164) — the strongest guarantee this fix could carry: the written selector re-parses andresolveSelectorElementIdsattributes the tween to exactly one element, not five. Locks the bug from recurring at the read side. - End-to-end coverage —
useEnableKeyframes.test.ts:470asserts against the actual product contract ("targets the selected sibling alone, not every element sharing its class"), which is what the user would notice if this regressed. - No new P0/P1 defects surface. CI is mixed only because the force-push cancelled prior runs; the in-flight Graphite mergeability check is expected to settle. No dependent stack invariants broken.
Grade: A. Fixed both what R1 asked for and, cleanly, the mirror defect Rames flagged — not by widening the ponytail but by factoring out a separately-named helper with its own docstring and tests. Design intent is now readable at both write-target contracts side-by-side.
Verdict: APPROVE.
— Review by Via
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
R2 reviewed at f04cdb7 (delta from 37140af).
Clean R2, no blockers. Every open R1 finding is resolved and the new existingTweenTargetSelector helper is a nicer design than my original suggestion — routing the replace-with-keyframes retargets through it preserves author intent (a tween the author aimed at a whole class group stays aimed at the group after promotion), where wiring writeTargetSelector into those paths would have silently narrowed them.
Resolved from R1:
useEnableKeyframes.ts—extendKeyframedTweenToPlayhead,promoteSetToKeyframes, andapplyArcKeyframeAtPlayheadnow go throughexistingTweenTargetSelector. The docblock explaining the write-vs-retarget distinction is load-bearing and easy to read.writeTargetSelectorbare-selector fallback semantics rewritten: null-on-DOM-miss (structural walk failed with a live document to check against) rather than handing back the input selector. Callers that can't afford to drop a user edit opt back in explicitly with?? selector, which makes the trade visible at each site (seecommitKeyframeAtTimeImplcomment). This is stronger than a dev-only warn — the null result forces callers to make the decision.- Deep-tree structural coverage: the new "hands identity back to a
data-hf-idancestor part way up a deep chain" test exercises exactly the mixed-identity walk the R1 nit was after. The invalid-CSS-selector-fall-through and detached-element-returns-null tests are welcome bonuses.
Observation, not blocking: existingTweenTargetSelector returns anim.targetSelector verbatim when !hasUnresolvedSelector, which means a legacy set written with a bare class (from before this whole fix) stays bare on promotion — the mis-attributed-to-siblings behavior survives for pre-existing tweens. This is consistent with the PR body's "retargets of an EXISTING tween are deliberately untouched" carve-out; flagging as the natural follow-up seam if a migration story for legacy tweens is on the roadmap.
Small polish: [...parent.children].indexOf(node) reads much cleaner than the old Array.prototype.indexOf.call form.
— Review by Rames D Jusso
The base branch was changed.
…d-keyframe-target fix(studio): target one element when adding a keyframe at the playhead
3 participants
What
Adding a keyframe to an element with no id wrote a tween that animates every one of its siblings.
Bugs fixed
buildStableSelectorhands back, so one add on a.groupelement wrotetl.to(".group", ...). That tween animates all five siblings, andresolveSelectorElementIdsreads it back as all five, collapsing their timeline rows into a single row. It survived a reload, so the written file stayed un-editable.replace-with-keyframesdeletes the tween and adds it back, so itstargetSelectoris a full rewrite of the target, and the paths inuseEnableKeyframesderived that string from the selection. Promoting a set on a tween already narrowed to#scene > div:nth-child(3)rewrote it back to.group, re-widening it onto every sibling.writeTargetSelectorreturned the selection's bare selector whenever the structural walk failed, including when a live DOM was there to check against. An element re-parented or removed between selecting and committing takes that path.Why
This is data loss in the user's own source file, not a display bug. One click silently rewrites five elements' animation and leaves the composition in a state Studio itself can no longer edit.
How
Add
writeTargetSelector, the write-side counterpart toselectorFromSelection(which must keep returning the exact stringfindTweenAtTimecompares against). It resolves the element's own identity to a selector addressing exactly one element, verifying every rung against the live document:#id, else[data-hf-id="..."], else the selection's own selector when it is already unique, else a:nth-childpath anchored on the nearest identifiable ancestor.Applied to the two paths that author a NEW tween: the no-animation branch of
useEnableKeyframes, andcommitKeyframeAtTimeImpl.For bug 2, the
replace-with-keyframespaths now keep the target the tween already writes (existingTweenTargetSelector), which is what eight sibling commit modules already do. Re-deriving it discarded whatever the author aimed at AND re-widened a tween a previous add had narrowed. The selection is the fallback only for a target the parser could not resolve statically.For bug 3,
writeTargetSelectorreturnsnullwhen a live DOM was available and no rung addressed one element. A failed walk against a live document is evidence, not the absence of it. Callers decide: the add-keyframe path already has a graceful no-animation fallback, and the paths that cannot afford to drop a user edit opt back in with?? selectorFromSelectionwhere the trade is visible in the call. The bare selector still comes back when there is no DOM to disambiguate against.Test plan
Studio suite green on this branch: 3042 passing, 0 failures. Typecheck, lint and format clean.
Not covered
useGestureCommitanduseGsapAnimationOpsalso pass a non-tween-derived target toreplace-with-keyframes. InuseGestureCommitthe tween was found by string equality against that same selector, so the two agree by construction;useGsapAnimationOpstakes the string as a parameter and its callers decide. Neither is re-widening today, so both are left alone.Known ceiling
The
:nth-childrung is computed against the preview DOM. GSAP andresolveSelectorElementIdsboth resolve against that same DOM so they agree, but a runtime that injects wrapper elements between authored ones would shift the index relative to the authored source.